[WIP] Enqueue only required frontend styles - V1 #6166

kanishkdudeja · 2018-04-13T08:44:34Z

This PR is a proof of concept for #5445. It only enqueues front-end styles for block types present in the current page/post.

Dependencies

Since core blocks aren't registered on server side (as of now, in master), it depends on [WIP] Update/build individual blocks #6087 to be merged first.

Version 1 (enqueues styles inside `<body>`)

This branch hooks a filter on the_content to get block types present in the page while stripping block comments from the HTML.

Since filters hooked to the_content are executed much later in the lifecycle (after actions hooked to wp_enqueue_scripts are executed), the stylesheets are being enqueued in the <body> section.

Version 2 (enqueues styles inside `<head>`)

#6200 reads the current post's content on the wp_enqueue_scripts action (much earlier than the_content). Therefore, that enables us to be able to enqueue required styles inside the <head> section.

An issue with that approach is that if a plugin adds a core block using a filter on the_content later, our code won't be able to enqueue styles for that block type since we would have read the post's content much earlier. Maybe we can try re-parsing the post's content to enqueue styles for any remaining block types after all filters on the_content have executed.

To Do

Make this work for dynamic blocks
Bundling for styles (Right now, styles for each block type are enqueued separately)
Provide a way to disable this functionality (meaning, enqueue all styles regardless of block types present in the page)
Provide a way to extend this functionality

Added a class "WP_Parsed_Block_Types_Registry" to manage the storage of block types parsed from the HTML (while stripping block comments)

…e HTML

Core block types parsed from the HTML may not contain 'core/'. Added a function gutenberg_prefix_core_namespace_if_not_found() to do the same.

…required-frontend-styles

This is work in progress and will be completely overhauled. Just committing it so that I don't lose it.

…required-frontend-styles

…ck comments

…e enqueued The new logic checks if the current requested web page is in the frontend. And if it's page or a post. If the answer is yes to both, it only enqueues styles for blocks which are present in the post/page.

…required-frontend-styles

diegoliv · 2018-04-13T13:59:29Z

Hey @kanishkdudeja I checked your work on this PR and on #5445 and I think that my goal on #5740 might be aligned with what you're doing here. Long story short, I think we could leverage the framework you're working for bundling frontend assets for blocks and also allow devs to add attribute based styles to be bundled into the final static css file that will be generated. What do you think?

aduth · 2018-04-13T15:01:21Z

lib/blocks.php

+ * @return string             Name of the block type, prefixed by the core namespace if needed.
+ */
+function gutenberg_prefix_core_namespace_if_not_found( $block_type ) {
+	$block_type = trim( $block_type );


Why would we expect leading or trailing whitespace?

We will not expect leading/trailing whitespace from where the function is being called right now (gutenberg_process_block_comment()).

But this function can also be called as a library function from other functions in future. So, I thought it might be a good idea to trim the input since a normalized block type cannot contain leading/trailing whitespace.

What do you think @aduth ?

aduth · 2018-04-13T15:02:29Z

lib/blocks.php

+ * @param  string $block_type Name of the block type.
+ * @return string             Name of the block type, prefixed by the core namespace if needed.
+ */
+function gutenberg_prefix_core_namespace_if_not_found( $block_type ) {


Maybe we could simplify / generalize the function name to reflect the fact that we want a normal/standard block type to work with:

gutenberg_normalize_block_type

Thanks. I also thought about this. Couldn't think of a proper name then.

Have renamed it now.

aduth · 2018-04-13T15:05:00Z

lib/blocks.php

+	// Only process the block comment if it's not a closing tag for a block. If it's a closing tag, we can just return. 
+	if ( preg_match( '/\/wp:/m', $block_comment ) !== 1 ) {
+
+		$match = Array();


We don't need to explicitly create a variable reference for preg_match to assign into. This line could be removed altogether and still work just as well.

Aside: I'd encourage lowercase array(). Confused me for a moment as I'm not used to seeing it this way, I thought it was some class special constructor.

Thanks. Removed the line altogether.

aduth · 2018-04-13T15:05:13Z

lib/blocks.php

+
+		$match = Array();
+
+		preg_match( '/wp:(.*?)\s+/m', $block_comment, $match);


Coding standards: Space before closing parenthesis.

Thanks. This has been resolved.

aduth · 2018-04-13T15:09:43Z

lib/blocks.php

+ *                         and so on. 
+ * @return string          Returns an empty string to preg_replace_callback() for each of the block comments
+ */
+function gutenberg_process_block_comment( $matches ) {


As an aside, might be worth tracking conversation at #5967, since at some point we may want to re-introduce a grammar for block rendering which doesn't rely as much on regular expressions. I think this is fine given the current implementation though.

Sure. I'll be tracking the conversation at #5967 now :)

aduth · 2018-04-13T15:16:10Z

lib/client-assets.php

 	$is_editor = ( 'enqueue_block_editor_assets' === current_action() );
+	$is_frontend = ! $is_editor;
+
+	$is_post_or_page = is_single() || is_page();


Would we just want to use is_singular here? Or are we intentionally excluding attachment type?

https://developer.wordpress.org/reference/functions/is_singular/

The original intention was to exclude attachment type. But it turns out is_single() returns true for attachment types as well.

Thanks for pointing this out. Have replaced this with: is_singular( array( 'post', 'page') )

aduth · 2018-04-13T15:18:07Z

lib/client-assets.php

@@ -770,10 +780,52 @@ function gutenberg_enqueue_registered_block_scripts_and_styles() {
 			wp_enqueue_script( $block_type->editor_script );
 		}
 	}
+
+	if ( $enqueue_only_required_styles ) {
+		add_action( 'the_content', 'enqueue_required_frontend_block_styles', 10 ); 


Priority 10 is default, so can be omitted altogether as an argument.

You'd mentioned wanting to support other plugins which modify the_content to revise blocks. Does that mean enqueueing needs to occur at a later priority (higher value)? What about the parsing to determine which blocks are present in content? Similarly later? Currently at 9, if a plugin would modify content at the default priority, I don't think WP_Parsed_Block_Types_Registry would reflect those modifications.

Yes. Thanks for pointing this out. I'm sorry this completely skipped my mind.

Now, gutenberg_process_block_comments runs with PHP_INT_MAX - 1 priority and enqueue_required_frontend_block_styles runs with PHP_INT_MAX priority since front-end styles can be enqueued once block types are parsed and saved in the registry.

aduth · 2018-04-13T15:18:36Z

lib/client-assets.php

+		if ( ! isset( $block_type ) ) {
+			// Log the error here
+		} else {
+			if ( isset( $block_type->style ) ) {


Collapse into elseif ?

Thanks. This has been resolved.

aduth · 2018-04-13T15:19:07Z

lib/client-assets.php

+		$block_type = $all_block_types_registry->get_registered( $block_type_name );
+
+		if ( ! isset( $block_type ) ) {
+			// Log the error here


How would you propose to log it?

Have logged it using trigger_error in the latest commit.

This function will do the following things if WP_DEBUG is set as true:

Print it in wp-content/debug.log file (if configured using WP_DEBUG_LOG).

Will show it as a notice on the post.

If WP_DEBUG is set as false (by default), it won't print the log anywhere.

aduth · 2018-04-13T15:21:47Z

lib/client-assets.php

+			// Log the error here
+		} else {
+			if ( isset( $block_type->style ) ) {
+				wp_enqueue_style( $block_type->style );


So because we enqueue in the_content, when exactly does the script get output? As part of late style printing in wp_footer? Would that trigger a flash of unstyled content for the block since the markup of the block is output before its stylesheet? If so, could we overcome this by forcing the output of stylesheets right here, before the content (echo immediately) ?

aduth · 2018-04-13T15:34:12Z

Broad thoughts I have are:

Maybe the registry is overkill, I know we’re mimicking the block registry, but it’s not clear why
Tying ourselves a bit closer to parsing content with regular expressions + string manipulation
Will we have issues with ordering of style enqueues + plugin filters

Renamed gutenberg_prefix_core_namespace_if_not_found() to gutenberg_normalize_block_type() Removed '$match = Array()' since we don't need to declare a variable for preg_match() Fixed syntax to suit coding standards

…blocks Our rendering algorithm for dynamic blocks was stripping block comments after rendering those dynamic blocks. Disabled that functionality since block comments are needed to find out block types present in the page later.

…algorithm

…required-frontend-styles

…gistry

get_last_error() function in Block Type Validator was referencing $errors as a local variable, whereas it should have referenced it as a class variable. Have fixed it in this commit.

Removed _doing_it_wrong notices from Block Type Validator class since the same are being called in the caller

…required-frontend-styles

…post

…ts and enqueueing frontend styles Earlier, functions for stripping block comments and enqueueing frontend styles were given 9 and 10 priority respectively. This didn't work well since plugins might append more blocks to content (through filters at higher priority). Therefore, have assigned PHP_INT_MAX - 1 and PHP_INT_MAX priority to these so that these can be run as late as possible

kanishkdudeja · 2018-04-16T17:55:12Z

Thanks for the elaborate and critical code review @aduth.

Maybe the registry is overkill, I know we’re mimicking the block registry, but it’s not clear why

The WP_Parsed_Block_Registry serves 2 purposes here:

Maintains some global state. So that functions which enqueue frontend styles for required block types can get a list of parsed block types after they are stripped from the HTML.
The same class can also be used by plugins later for any custom needs. If a plugin needs to get a list of block types parsed from the current post/page, they can use the same class.

Tying ourselves a bit closer to parsing content with regular expressions + string manipulation

Yes, I felt the same. But we're already using similar regular expressions to parse dynamic block patterns to be able to render them on the server-side. So, I thought this should be okay for now.

As you suggested, I'm following the conversation on #5967 but do let me know if you feel we should use the PEG Parser to be able to parse block types while stripping block comments from the code.

Will we have issues with ordering of style enqueues + plugin filters

I don't think we will. That is a possibility in #6200 (V2).

Since we are enqueuing required styles in the <body> section, parsing block types (while stripping block comments) is done through a filter (with priority PHP_INT_MAX) hooked to the_content. Therefore, unless plugins attach filters with the same priority (maximum possible), we should be able to parse all block types (even those added by plugins) before we start to enqueue stylesheets for those block types.

…f it's not registered

…required-frontend-styles

A comment describing the priority for a filter has now been moved to just above the add_filter() function so that it's more intuitive while reading.

Earlier, we were parsing block types from the HTML and saving them to the registry even for pages like category / archive pages (which contain multiple posts). Have disabled this functionality for such pages now since the registry is only needed for intelligent enqueueing of assets (which is needed for single post / page types as of now).

aduth · 2018-09-13T19:26:29Z

This pull request appears to have languished and will not be easily reconciled with master. Please feel free to reopen and rebase against the current master, or open a new pull request.

Kanishk Dudeja added 15 commits April 8, 2018 21:27

Added class for managing the storage of block types parsed from the HTML

c580ac9

Added a class "WP_Parsed_Block_Types_Registry" to manage the storage of block types parsed from the HTML (while stripping block comments)

Added @SInCE version to WP_Parsed_Block_Types_Registry class

27c4964

Added logic to find block type while stripping block comments from th…

b91904b

…e HTML

Added a function to prefix the core namespace to the block type

855bb0e

Core block types parsed from the HTML may not contain 'core/'. Added a function gutenberg_prefix_core_namespace_if_not_found() to do the same.

Merge branch 'master' into add/enqueue-only-required-frontend-styles

30a1438

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

1e27566

…required-frontend-styles

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

d9afec7

…required-frontend-styles

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

da22012

…required-frontend-styles

WIP: Just adds a function gutenberg_enqueue_required_block_styles()

d2d119e

This is work in progress and will be completely overhauled. Just committing it so that I don't lose it.

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

f17746c

…required-frontend-styles

Added documentation blocks to functions responsible for stripping blo…

76752c1

…ck comments

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

64c4e48

…required-frontend-styles

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

7763fae

…required-frontend-styles

Added documentation for enqueue_required_frontend_block_styles()

c89713a

gziolo mentioned this pull request Apr 13, 2018

Proposal: add "assets_callback" to "register_block_type" #5705

Closed

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience Framework Issues related to broader framework topics, especially as it relates to javascript labels Apr 13, 2018

kanishkdudeja mentioned this pull request Apr 13, 2018

Bundling Front-end Assets #5445

Closed

aduth reviewed Apr 13, 2018

View reviewed changes

Kanishk Dudeja added 6 commits April 14, 2018 14:37

Removed trailing and unintended spaces using EditorConfig plugin

e3275b6

Syntax fixes and renamed function which normalized block type

4beb6a1

Renamed gutenberg_prefix_core_namespace_if_not_found() to gutenberg_normalize_block_type() Removed '$match = Array()' since we don't need to declare a variable for preg_match() Fixed syntax to suit coding standards

Merge branch 'master' into add/enqueue-only-required-frontend-styles

4715f77

Fixed linting errors reported by PHP CodeSniffer

872f4c3

Added a class WP_Block_Type_Validator for validating block types

fa50524

Kanishk Dudeja added 7 commits April 15, 2018 22:57

Changed text in a comment to reflect the new dynamic block rendering …

d0ca801

…algorithm

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

ac3a60a

…required-frontend-styles

Added a check to only add unique block types to parsed block types re…

a56cd90

…gistry

Fixed unit tests for dynamic block rendering and stripping block types

173d19b

Fixed spaces around Concat operator in unit tests

2e271ea

Fixed an error in get_last_error() function in Block Type Validator

f919a9f

get_last_error() function in Block Type Validator was referencing $errors as a local variable, whereas it should have referenced it as a class variable. Have fixed it in this commit.

Removed _doing_it_wrong notices from Block Type Validator class

a97737e

Removed _doing_it_wrong notices from Block Type Validator class since the same are being called in the caller

kanishkdudeja mentioned this pull request Apr 16, 2018

[WIP] Enqueue only required frontend styles - V2 #6200

Closed

4 tasks

Kanishk Dudeja added 6 commits April 16, 2018 17:04

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

0db02bd

…required-frontend-styles

Used is_singular() to check if current requested page is a page or a …

8d6ae65

…post

Coding standard: Added space before closing the array bracket

d162365

Added $added_from_version field to Errors in Block type validator

37d8123

Improved documentation for get_errors() function

e58d51f

Kanishk Dudeja added 7 commits April 17, 2018 00:50

Added error logging for - Not able to enqueue styles for block type i…

a938396

…f it's not registered

Merge remote-tracking branch 'upstream/master' into add/enqueue-only-…

b0aab53

…required-frontend-styles

Merge branch 'master' into add/enqueue-only-required-frontend-styles

6c79ec4

Moved a comment to a better suited place for it

9e2696b

A comment describing the priority for a filter has now been moved to just above the add_filter() function so that it's more intuitive while reading.

Merge branch 'master' into add/enqueue-only-required-frontend-styles

b9bc4d4

Improved documentation block for WP_Parsed_Block_Types registry

f6e5750

gziolo mentioned this pull request May 14, 2018

Loading front-end assets #6450

Closed

gziolo added the [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible label Jul 10, 2018

aduth closed this Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Enqueue only required frontend styles - V1 #6166

[WIP] Enqueue only required frontend styles - V1 #6166

kanishkdudeja commented Apr 13, 2018 •

edited

Loading

diegoliv commented Apr 13, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 15, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 14, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 14, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 14, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 15, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 16, 2018

aduth Apr 13, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 16, 2018 •

edited

Loading

aduth Apr 13, 2018

kanishkdudeja Apr 14, 2018

aduth Apr 13, 2018

kanishkdudeja Apr 16, 2018

aduth Apr 13, 2018

aduth commented Apr 13, 2018

kanishkdudeja commented Apr 16, 2018

aduth commented Sep 13, 2018


		$match = Array();

		preg_match( '/wp:(.*?)\s+/m', $block_comment, $match);

[WIP] Enqueue only required frontend styles - V1 #6166

[WIP] Enqueue only required frontend styles - V1 #6166

Conversation

kanishkdudeja commented Apr 13, 2018 • edited Loading

Dependencies

Version 1 (enqueues styles inside <body>)

Version 2 (enqueues styles inside <head>)

To Do

diegoliv commented Apr 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanishkdudeja Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 13, 2018

kanishkdudeja commented Apr 16, 2018

aduth commented Sep 13, 2018

kanishkdudeja commented Apr 13, 2018 •

edited

Loading

Version 1 (enqueues styles inside `<body>`)

Version 2 (enqueues styles inside `<head>`)

kanishkdudeja Apr 16, 2018 •

edited

Loading